Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFC FS 1126 Allow lower-case DU cases when RequireQualifiedAccess is specified #13432

Merged
merged 5 commits into from
Jul 11, 2022
Merged

RFC FS 1126 Allow lower-case DU cases when RequireQualifiedAccess is specified #13432

merged 5 commits into from
Jul 11, 2022

Conversation

edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Jul 3, 2022 •
edited by dsyme
Loading

@edgarfgp edgarfgp changed the title DRAFT Allow lower-case DU cases when RequireQualifiedAccess is specified Allow lower-case DU cases when RequireQualifiedAccess is specified Jul 3, 2022
@edgarfgp edgarfgp marked this pull request as ready for review July 4, 2022 06:44
dsyme
dsyme reviewed Jul 4, 2022
View reviewed changes
src/Compiler/Checking/CheckDeclarations.fs Outdated
let hasRequireQualifiedAccessAttribute = HasFSharpAttribute cenv.g cenv.g.attrib_RequireQualifiedAccessAttribute tycon.Attribs

if not hasRequireQualifiedAccessAttribute then
CheckUnionCaseName cenv id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need these parts of CheckUnionCaseName because the compiled form must generate a type and enum case for these - better to pass in hasRequireQualifiedAccessAttribute to that routine?

        if name = "Tags" then
            errorR(Error(FSComp.SR.tcUnionCaseNameConflictsWithGeneratedType(name, "Tags"), id.idRange))

        CheckNamespaceModuleOrTypeName g id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated based on your suggestion.

dsyme
dsyme reviewed Jul 4, 2022
View reviewed changes
...Sharp.Compiler.ComponentTests/Conformance/UnionTypes/LowercaseWhenRequireQualifiedAccess.fsx

[<RequireQualifiedAccess>]
type DU1 =
| a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case with a dodgy lower case name that is not a valid type name for .NET, e.g.

type DU =
    | ``not.allowed``

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more testing. Thanks for pointing this out .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vzarytovskii I was wondering if should put this behind a preview flag ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's put it under preview version.

@edgarfgp edgarfgp force-pushed the allow-lower-case-du-cases-when-require-qualified-access-is-specified branch from 811eac2 to a9bf35c Compare July 6, 2022 20:20
@edgarfgp
Copy link
Contributor Author

edgarfgp commented Jul 7, 2022

@dsyme this is ready for another review . Added more testing and put the feature under the preview.

vzarytovskii
vzarytovskii reviewed Jul 7, 2022
View reviewed changes
...arp.Compiler.ComponentTests/Conformance/UnionTypes/E_LowercaseWhenRequireQualifiedAccess.fsx Outdated
Comment on lines 1 to 3
// #Conformance #TypesAndModules #Unions
// This testcase verifies that lower-case discriminated union are not allowed
//<Expects status="error"></Expects>
Copy link
Member

@vzarytovskii vzarytovskii Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: those are actually not required for newly written tests, they are there for the tests which were auto-migrated from the old framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool I will remove them :)

@auduchinok
Copy link
Member

auduchinok commented Jul 7, 2022 •
edited
Loading

Should this change somehow take languages without notion of lower/uppercase into account?
See #4697, #9199, #9186 for the context.

Update: it seems they should just work already.

@vzarytovskii vzarytovskii merged commit b8aaf2d into dotnet:main Jul 11, 2022
@vzarytovskii
Copy link
Member

@edgarfgp thank you a lot for this addition! Sorry it took long to merge it.

@edgarfgp
Copy link
Contributor Author

@vzarytovskii No worries . I already looking for the next contribution to help the compiler 😀

@NinoFloris NinoFloris mentioned this pull request Jul 12, 2022
Closed
vzarytovskii added a commit that referenced this pull request Jul 12, 2022
@dotnet-bot @dsyme
@psfinaki @nojaf @edgarfgp @Happypig375 @vzarytovskii @KevinRansom
Merge main to release/dev17.4 (#13499)
873eefc 
* ValRepInfoForDisplay added for improved quick info for functions defined in expressions

* Update

* Update QuickInfoTests.fs

* Update QuickInfoTests.fs

* Update

* add identifier analysis script (#13486)

* add identifier analysis script

* add identifier analysis script

* Update fantomas alpha 11 (#13481)

* Allow lower-case DU cases when RequireQualifiedAccess is specified (#13432)

* Allow lower-case DU cases when RequireQualifiedAccess is specified

* Fix PR suggestions and Add more testing

* Protect feature under preview version

* Add a NotUpperCaseConstructorWithoutRQA warning to be raised in lang version preview

* Fix formatting

* regularize some names (#13489)

* normalize some names

* format code

* fix build

* Subtraction of two chars, new conversions, and fixes for dynamic operator invocations and QuotationToExpression (#11681)

Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
Co-authored-by: Don Syme <dsyme@users.noreply.github.com>

* Mark backing fields as CompilerGenerated (fixes serialization of fields in FSI multiemit) (#13494)

Co-authored-by: Don Syme <donsyme@fastmail.fm>
Co-authored-by: Peter Semkin <petersemkin@duck.com>
Co-authored-by: Don Syme <dsyme@users.noreply.github.com>
Co-authored-by: Florian Verdonck <florian.verdonck@outlook.com>
Co-authored-by: Petr Semkin <psfinaki@users.noreply.github.com>
Co-authored-by: Edgar Gonzalez <edgar.gonzalez@fundourselves.com>
Co-authored-by: Hadrian Tang <hadrianwttang@outlook.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
@edgarfgp edgarfgp changed the title Allow lower-case DU cases when RequireQualifiedAccess is specified RFC FS 1126 Allow lower-case DU cases when RequireQualifiedAccess is specified Aug 10, 2022
@dsyme dsyme mentioned this pull request Aug 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsyme dsyme dsyme left review comments

@vzarytovskii vzarytovskii vzarytovskii approved these changes

Assignees
No one assigned
Labels
None yet
Projects
F# Compiler and Tooling
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@edgarfgp @auduchinok @vzarytovskii @dsyme